-
Notifications
You must be signed in to change notification settings - Fork 1.2k
mspm0: read_reset_cause() #4831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I don't understand those ci/build failures. This builds just fine locally. |
|
Ah, I see the problem. For some MSPM0 chips the |
Maybe better to patch the PAC in that case? |
|
There's other funny stuff going on with the MSPM0L13xx PAC -- it has WWDT0 and WWDT1 reset causes but there's only one WWDT0 defined for e.g MSPM0L1306... And some of the PAC reset cause values don't line up with the MSPM0 tech reference either. I'll investigate some more. |
|
I couldn't think of a better way to do this. Even if I fixed the reset cause Id enum in the PAC, it still would have a different set of values for the G-series, which would make for a horrific |
In this case if there are enough differences I would just define 2 enums and pick based on chip family with cfg. |
It's one extra enum variant for G-series chips. Everything else is the same as far as I can tell. I can use cfg to make that one enum variant only preset for G series. Are you okay with using the hard-coded values instead of the PAC enum values? |
Something like is fine for enum definitions. But I'm not sure we want to make it repr(u8) |
Alternatively, we don't define values for |
That's fine and probably preferable (we don't guarantee ABI stability of values) |
|
Spurious job failure? |
C1104 failed to build. Probably that missing Id field you mentioned? |
Ah crap, yes. I need to figure out how to do a build against every variant to catch that. |
|
This depends on mspm0-rs/mspm0-data#18 to add the missing PAC enum for the C-series but will still need further fixes since there are subtle differences in the PAC enum between series. |
i509VCB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pending on mspm0-data merge.
|
Still have to edit this to make two of the variants optional on c110x. |
| /// | ||
| /// If the reset cause is not recognized, an `Err` containing the raw value is returned. | ||
| #[must_use = "Reading reset cause will clear it"] | ||
| pub fn read_reset_cause() -> Result<ResetCause, u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comment is relating to return value. We do exhaustively measure each value hardware can produce, so we could mark other values as unreachable.
unreachable_unchecked is technically valid but would cause undefined behavior when new chips are added and this code isn't updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather err (haha) on the side of caution and keep the Err return to avoid undefined behavior. A runtime panic is better than undefined behavior.
i509VCB
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible can you squash the commits?
This adds a read_reset_cause function that returns an enum value derived from the SYSCTL.RSTCAUSE register. Based on the work by @charlesbmi in embassy-rs#4732.
This adds a
read_reset_causefunction that returns an enum value derived from the SYSCTL.RSTCAUSE register.Based on the work by @charlesbmi in #4732.
Example usage code tested on-device: